Workflow [PR], commit [0eae438]

Codebase Consistency Review — Altinity/ClickHouse PR #1721

Scope. PR adds src/Storages/HybridSegmentPruner.{h,cpp} (≈490 LOC) plus wiring in StorageDistributed to skip Hybrid-engine segments when (PREWHERE ∧ WHERE ∧ segment_predicate) is provably unsatisfiable. The pruner does AST walking, bounded DNF expansion, per-column typed range/IN intersection over Core/Range, and constant folding.

User flag: "AFAIR there was already code for pruning things and detect always false expression." That intuition is correct.

Executive summary

One High consistency finding, one Medium.

The pruner reimplements the core of KeyCondition — RPN-style satisfiability over typed per-column ranges, with alwaysFalse / checkInHyperrectangle already exposing exactly the "is this predicate empty over the universe of these columns" question. KeyCondition is not tied to the sorting key: it is constructed with an arbitrary key_column_names list and is already used that way by PartitionPruner, MergeTree min-max index, secondary skip indices, and Set index. The "key" in its name is historical.

Findings

Finding 1 — Reinvents KeyCondition::alwaysFalse / checkInHyperrectangle over arbitrary columns (Severity: High, Category: Duplicated functionality / standard mechanism bypassed)

New code: src/Storages/HybridSegmentPruner.cpp:362-455 — canPruneHybridSegment + the supporting ColumnDomain, applyAtomToDomains, branchIsUnsatisfiable, extractAtom, comparison-op enum, range/IN intersection.

Existing precedent:

• src/Storages/MergeTree/KeyCondition.h:147 / KeyCondition.cpp — alwaysFalse() does exactly the "predicate provably unsatisfiable" check via 3-state RPN lattice.
• src/Storages/MergeTree/KeyCondition.h:113-117 — checkInHyperrectangle(ranges, types) returns a BoolMask; calling it with Range::createWholeUniverse() per column answers "is the predicate empty over the whole domain of these columns" — i.e. the exact question this PR's pruner asks.
• src/Storages/MergeTree/PartitionPruner.{h,cpp} — same pattern at the partition level: build a KeyCondition over partition-key columns and let it decide skip.
• KeyCondition is already constructed over arbitrary column sets, not just the sorting key (used by min-max index, set/bloom_filter/tokenbf skip indices, projection key analysis). The constructor takes key_column_names as a free list.
• KeyCondition already understands: and/or/not, =/</<=/>/>=/in/notIn/like, IN with sets, notEquals, isNull, side-swapping (5 < x ↔ x > 5), monotonic function chains, type coercion via convertFieldToType, NULL handling, Tuple IN-lists. The new pruner supports a strict subset and gives up on notEquals, NOT IN, monotonic functions.

Analysis. The new code is essentially a hand-rolled, weaker KeyCondition specialized to the Hybrid case. Concretely:

• ColumnDomain { Range range; optional<set<Field>> allowed; } ≈ KeyCondition's per-column hyperrectangle plus Set element list.
• applyAtomToDomains ≈ KeyCondition RPN reduction step.
• branchIsUnsatisfiable ≈ alwaysFalse per disjunct.
• The bounded DNF cap (MAX_OR_GROUPS=2, MAX_TOTAL_BRANCHES=4) is a workaround for not having KeyCondition's RPN evaluator, which handles arbitrary OR depth in linear time without DNF blow-up.
• The comment at HybridSegmentPruner.cpp:64-66 ("Range::contains has buggy effectively-always-false semantics, use intersectsRange") is a smell: if Range::contains is broken, fix it in Core/Range.cpp rather than route around it in a new caller. KeyCondition already does the right thing here.

The "fail-open on exception" wrapper (catch (...) return false) is also non-standard — the rest of MergeTree's pruning paths log and surface, and rely on KeyCondition returning BoolMask::consider_all() for unknowns rather than swallowing exceptions.

Recommendation.

1. Replace the body of canPruneHybridSegment with: build an ActionsDAG (or use the existing query_info.filter_actions_dag produced by the planner) restricted to columns present in hybrid_columns; construct a KeyCondition with those columns as key_column_names; return kc.alwaysFalse(). That gives free support for notEquals, NOT IN, monotonic functions, NULLs, all common functions, and removes the DNF-size cap.
2. If a filter ActionsDAG is not available at this call site (legacy non-analyzer path), use KeyCondition's AST constructor — it accepts a Block of available columns.
3. Drop Core/Range::contains workaround; if it's wrong, fix it in Core/Range.cpp.
4. Delete HybridSegmentPruner.{h,cpp} and the gtest; keep only the StorageDistributed wiring (verdict struct, snapshot-frozen watermark, the read() integration) which is genuinely Hybrid-specific.

If for some reason KeyCondition cannot be reused directly here (e.g. the analyzer hasn't produced a DAG yet for this code path), the right move is still not a parallel implementation — it's to build the DAG via buildFilterDAG / KeyCondition's AST entry point, the same way PartitionPruner does.

Confidence: High that overlap is substantial; medium that direct delegation is mechanically clean (depends on whether the AST is post-analyzer at this point — worth a 30-minute spike before committing to the refactor).

Finding 2 — JOIN-detection and column-by-shortName lookup duplicate analyzer/planner logic (Severity: Medium, Category: Standard mechanism bypassed)

New code: StorageDistributed.cpp:690-709 walks select->tables()->children looking for table_join to bail out on JOINs; HybridSegmentPruner.cpp:212 uses ASTIdentifier::shortName() and matches against hybrid_columns to decide which atoms refer to the Hybrid table.

Existing precedent.

• The analyzer already resolves columns to specific tables via ColumnNode / QueryTreeNode. query_info.filter_actions_dag already contains only inputs that resolve to this storage. Walking the AST and comparing shortName() is the pre-analyzer way and is exactly the kind of fragility the analyzer was designed to remove — Test 9 in the new SQL test (alias shadowing ts) is a symptom that the AST-level shortName match can't distinguish a Hybrid column from a same-named alias.
• JOIN detection: getQueryProcessingStage and the existing distributed code paths already know whether a query has a JOIN; recomputing it by walking tables()->children adds a third source of truth.

Recommendation. Once Finding 1 is applied (KeyCondition over a filter DAG), the JOIN guard and shortName matching both fall away — the DAG already has only this storage's columns and is built by the analyzer with proper scoping. If the legacy non-analyzer path must be supported separately, factor the JOIN check to the existing utility used elsewhere in StorageDistributed rather than inlining a new walker.

Confidence: Medium. Worth verifying Test 9 still passes via the DAG-based path before declaring the guard unnecessary.

Checked but not flagged

• substituteHybridWatermarks extraction to a static method (was a lambda) — fine, it's a real refactor that enables reuse between getQueryProcessingStage and read.
• HybridSnapshotData / StorageSnapshot::Data pattern — correct use of the existing extension point.
• getClusterWithMultipleShards({}) to signal "all shards skipped" — matches the optimize_skip_unused_shards idiom called out in the PR's own comment; consistent.
• Empty-cluster + surviving-segments local-plan fallback (StorageDistributed.cpp:914-942) — Hybrid-specific, no obvious precedent to reuse.

Bottom line

The user's suspicion is correct: KeyCondition::alwaysFalse (and its hyperrectangle variant) is the "already exists" they were thinking of, and PartitionPruner is the canonical example of using it for skip-decisions over a non-sort-key column set. The Hybrid pruner can shrink to a thin adapter that builds the column list and the filter DAG and delegates. That removes ~450 LOC, the DNF-size cap, the Range::contains workaround, and the AST-level alias-shadowing risk — and gains support for !=, NOT IN, monotonic functions, and NULL semantics for free.

Review report: #1721 — "Antalya 26.1 — hybrid: added support for segment pruning"

Summary

Adds a conservative satisfiability pruner (HybridSegmentPruner, ~450 LoC) and wires it into StorageDistributed so that Hybrid segments whose (PREWHERE ∧ WHERE ∧ segment_predicate) is provably empty are skipped. Watermark values are frozen on the storage snapshot to keep the verdict consistent across getQueryProcessingStage and read. The design has a clear fail-open contract, a focused test set, and a sound on-paper concurrency story. However, there are several plausibly silent-data-loss paths (analyzer ineffectiveness, UNION-ALL, subquery WHERE leakage, non-strict IN coercion), an architectural duplication of KeyCondition, a fail-open gap around substituteHybridWatermarks, and a bypass of ClusterProxy::executeQuery that may break non-loopback deployments. Verdict: request changes.

Reviewed scope

• Diff: PR Antalya 26.1 - hybrid: added support for segment pruning #1721 (mkmkme/antalya-26.1/hybrid/segment-pruning → antalya-26.1)
• Files changed: 7 (+1087 / −62)
• Main moving parts:
  • src/Storages/HybridSegmentPruner.{h,cpp} — new bounded-DNF + per-column typed range/IN satisfiability check
  • src/Storages/StorageDistributed.{h,cpp} — HybridSnapshotData, HybridPruningVerdict, computeHybridPruningVerdict, lifted substituteHybridWatermarks, new local-plan branch in read()
  • src/Storages/tests/gtest_hybrid_segment_pruner.cpp — 5 unit cases
  • tests/queries/0_stateless/03646_hybrid_segment_pruning.{sql,reference} — 9 integration scenarios

Blockers

B1. Pruner appears to be a no-op on the analyzer path

• Sources: deep-audit, tests, architecture
• Files/lines: StorageDistributed.cpp computeHybridPruningVerdict (~682–688)
• Evidence: select_for_pruning is taken from query_info.query via as<ASTSelectQuery>() / first child of ASTSelectWithUnionQuery. With allow_experimental_analyzer=1 the Planner uses query_info.query_tree; query_info.query is generally nullptr until later distributed rewriting. None of the SQL tests sets allow_experimental_analyzer explicitly, but the integration test asserts an analyzer-style plan (ReadFromPreparedSource (_exact_count_projection)), so this needs verification — there is at least an internal inconsistency that warrants explicit coverage on both enable_analyzer=0 and =1.
• Impact: feature silently does nothing under the now-default analyzer path → marketed optimization absent in production.
• Proposed fix: extract the predicate from query_info.query_tree (e.g. QueryNode::getWhere) for the analyzer path and only fall through to the AST path when query_tree is null. Add explicit tests that pin enable_analyzer to both values and assert the pruned plan shape.

B2. New "base pruned + additional segments survive" branch bypasses ClusterProxy::executeQuery

• Sources: ux-contract, architecture, deep-audit
• Files/lines: StorageDistributed.cpp ~914–942
• Evidence: when the cluster ends up empty, the code constructs local plans with createLocalPlan(..., shard_num=0, shard_count=1, ...) regardless of whether the surviving additional segment is local. ClusterProxy::executeQuery's additional_query_infos block normally handles distributed/remote setup. Tests use only 127.0.0.1:9000 so a real remote segment is never exercised through this branch.
• Impact: for non-loopback remotes (the realistic deployment), a base-pruned query may silently read no remote data and return wrong/empty results.
• Proposed fix: extract the local-plan loop in ClusterProxy::executeQuery into a shared helper and reuse it; or route surviving segments through the existing path with an empty cluster, fixing whatever in updateSettingsAndClientInfoForCluster actually trips on zero shards (the comment claims a crash there — verify; if confirmed, fix at the source rather than fork the loop).

B3. UNION ALL: only the first SELECT feeds the pruner

• Sources: ux-contract, correctness
• Files/lines: StorageDistributed.cpp ~686–688
• Evidence: select_for_pruning = union_query->list_of_selects->children.front()->as<ASTSelectQuery>(). Other arms are ignored.
• Impact: SELECT … FROM ht WHERE ts > '…' UNION ALL SELECT … FROM ht lets the first arm's narrow WHERE prune segments needed by a broader second arm → silent missing rows.
• Proposed fix: when the query is a UNION, conservatively skip pruning unless there is a single arm.

B4. Subquery / outer-WHERE leakage with column-name reuse

• Sources: ux-contract, correctness, security
• Files/lines: computeHybridPruningVerdict ~705–708; extractAtom use of ident->shortName()
• Evidence: select_for_pruning is fixed to the outer SELECT regardless of where the Hybrid table actually sits. For SELECT … FROM (SELECT … FROM hybrid) WHERE ts > X, the outer WHERE references the subquery's output columns. Because pruner column matching is purely lexical (shortName() against hybrid_columns), if names collide the outer WHERE silently feeds segment pruning. Test 9 only exercises the case where the outer WHERE is absent — it does not actually probe the dangerous shape.
• Impact: silent data loss for any nested-select pattern that happens to reuse a Hybrid column name in its output schema.
• Proposed fix: skip pruning unless query_info.query is a direct SELECT … FROM <hybrid> (no intervening subquery), or pass an explicit "this column resolves to this storage" check rather than name-only matching. Add a regression test of the form SELECT count() FROM (SELECT toDateTime('2025-11-01') AS ts, value FROM pruning_t) WHERE ts <= '2025-08-01' and require all 4 rows.

B5. evalTupleAndCoerce uses non-strict coercion despite the comment claiming "strict"

• Sources: correctness, security
• Files/lines: HybridSegmentPruner.cpp ~155
• Evidence: code calls convertFieldToType (non-strict); guard only catches Null results. The comment a few lines above says convertFieldToTypeStrict will conservatively reject imprecise/lossy coercions — but that function is not the one called. Scalar RHS uses the strict variant correctly via evalAndCoerce.
• Impact: lossy IN-list coercion (Decimal precision rounding, float→int truncation) can produce a coerced value that intersects empty against a segment predicate's IN-set when the original value would not → spurious prune → silent data loss. Direction matters for soundness.
• Proposed fix: use convertFieldToTypeStrict (with the inferred element type from the folded tuple) and bail (return {}) on null. Update or remove the misleading comment.

B6. substituteHybridWatermarks exceptions escape the fail-open guard

• Sources: ux-contract, lifetime, concurrency
• Files/lines: computeHybridPruningVerdict check lambda ~711–719
• Evidence: substituteHybridWatermarks runs before canPruneHybridSegment (whose try/catch(...) provides the fail-open). It throws BAD_ARGUMENTS on null watermarks or missing keys. computeHybridPruningVerdict itself has no guard, so the throw escapes both getQueryProcessingStage and read.
• Impact: a Hybrid table whose hybridParam was not yet set via ALTER MODIFY SETTING now fails inside stage planning rather than degrading gracefully; the HybridSegmentPruner.h header explicitly promises "exceptions … keep the segment."
• Proposed fix: wrap the body of the check lambda (or computeHybridPruningVerdict itself, excluding cancellation) in a try/catch returning false; log a warning so misconfiguration remains visible.

Major issues

M1. Reinvention of KeyCondition/alwaysFalse

• Sources: architecture
• Files/lines: HybridSegmentPruner.cpp (entire file)
• KeyCondition already does typed RPN, AND/OR, range/IN domains, NOT, and alwaysFalse() with battle-tested semantics. The new pruner is a lower-fidelity parallel implementation that will not benefit from upstream fixes. Strongly consider building a KeyCondition from the combined predicate over the Hybrid columns; if there is a fundamental reason it can't be reused (e.g. need to handle multi-column key predicates differently), the rationale must be documented in the PR.

M2. Verdict computed twice with no enforced binding between the two sites

• Sources: architecture, ux-contract, perf
• Files/lines: getQueryProcessingStage (~543–553), read (~776–778)
• The two calls "agree" only if both receive the same storage_snapshot and query_info is not mutated between them (e.g. WHERE constant-folding during re-analysis can flip the verdict; optimized_cluster mutation persists across re-analysis). Cache the verdict on SelectQueryInfo (or attach it to a per-query state object) and assert/reuse rather than recompute.

M3. HybridSnapshotData belongs on the per-query info, not the storage snapshot

• Sources: architecture, concurrency
• Watermark values are query-time parameters, not storage structure. Hanging them off StorageSnapshot::Data couples a per-query value to an object designed to outlive a query and possibly be reused (query_info.initial_storage_snapshot). The current dynamic_cast + fallback pattern silently breaks the freezing invariant when bypassed. Move to SelectQueryInfo or a dedicated per-query Hybrid context.

M4. Watermark deserialization silently accepts logically invalid dates

• Sources: security
• Files/lines: substituteHybridWatermarks ~636–639
• data_type->getDefaultSerialization()->deserializeWholeText for Date/DateTime calls into makeDayNum with default_error_day_num=0; values like '2025-13-01' are syntactically parseable but logically invalid → silently coerced to 1970-01-01. The substituted predicate then becomes ts <= 1970-01-01 (or similar) — for cold-segment predicates this looks unsatisfiable for typical user queries → segment pruned → silent missing rows.
• Proposed fix: validate via tryDeserializeWholeText and reject explicitly when the parser does not consume the input fully or returns a sentinel; or stricter format readDateTextFallthrough.

M5. JOIN guard relies on raw AST shape; analyzer path not fully covered

• Sources: architecture, deep-audit, security
• Files/lines: computeHybridPruningVerdict ~693–704
• AST-level walk of ASTTablesInSelectQueryElement::table_join is fine for the legacy path; under the analyzer the synthesized ASTSelectQuery may not preserve the same shape. The risk compounds with B1 — if the analyzer path is "fixed" to actually run pruning, it must also detect joins via the query tree.
• Proposed fix: add query_tree-level join detection (JoinNode) in addition to or instead of the AST walk.

M6. Pruner column matching by lexical shortName() is a load-bearing implicit contract

• Sources: architecture, correctness, security
• extractAtom uses ident->shortName() to match Hybrid columns; correctness depends on the caller stripping JOIN-side predicates. The contract is documented in the header but not enforced — a future caller that forgets the precondition silently mis-prunes. Strengthen by either passing a "no joins here" assertion / bool has_join parameter, or by carrying resolved column origins instead of names.

Minor issues / improvements

• Range::contains(FieldRef) is genuinely buggy in src/Core/Range.cpp (rightThan is logically inverted — [1,5].contains(3) returns false). The PR's workaround via intersectsRange(Range(point)) is correct, but the underlying API trap should be fixed or marked [[deprecated]] rather than left alongside the comment in this PR. (perf-ockham-headers-docs)
• canPruneHybridSegment's catch(...) swallows cancellation/bad_alloc. Consider re-raising Exception with code QUERY_WAS_CANCELLED and std::bad_alloc. (concurrency, lifetime)
• collectConjuncts / collectDisjuncts recurse with no depth limit; catch(...) does not catch SIGSEGV from stack overflow. Practical risk is low because the SQL parser bounds depth, but a max_depth parameter is cheap insurance. (lifetime)
• finalizeDomain idiom return !(domain.empty = true) is correct but obfuscated; replace with domain.empty = true; return false;. (correctness)
• branchIsUnsatisfiable for an empty branch_atoms returns false (tautology) — correct, but worth an explicit early-return + comment to avoid the cartesian product producing an unintentionally-empty branch in future. (ux-contract)
• The // TODO: check logic comment near the surviving-segments arithmetic is now actively touched by this PR; resolve or remove. (perf-ockham)
• HybridSnapshotData inherits StorageSnapshot::Data — verify Data has a virtual destructor (likely yes; confirm). (lifetime)
• Mandatory atoms are re-evaluated up to MAX_TOTAL_BRANCHES times during the cartesian product; pre-fold them once to keep this scalable if the bounds are ever raised. (perf)
• The 7th createLocalPlan argument is mis-commented (/*has_missing_objects=*/false — actual parameter is build_logical_plan). (deep-audit)

Needs verification

• Whether query_info.query is non-null on the analyzer/Planner path at the time computeHybridPruningVerdict runs (root of B1).
• Whether ClusterProxy::executeQuery truly cannot be called with an empty cluster (B2 motivation), or whether the empty-cluster crash can be fixed in-place to avoid the duplicated path.
• Whether tryEvaluateConstantExpression for a Tuple literal exposes per-element source types so strict tuple-element coercion is achievable (B5 fix).
• Whether segments and base_segment_predicate on StorageDistributed are mutable post-construction (e.g. via ALTER); both getQueryProcessingStage and read access them lock-free (concurrency).
• Whether identifier shadowing / alias paths can yield an ASTIdentifier matching a Hybrid column name in query_info.query before analyzer alias substitution (B4, security F6, tests Update README.md #10).
• Concrete repro of the date-sentinel issue (M4): set hybrid_watermark_*='2025-13-01', observe pruning verdict.
• That createLocalPlan always returns non-null (*plans.front() would be UB otherwise).

Suggested commit / diff split

• Core change (one PR):
  1. HybridSegmentPruner (or, preferred, a thin wrapper over KeyCondition::alwaysFalse).
  2. StorageDistributed wiring + verdict caching on SelectQueryInfo.
  3. Tests covering analyzer & legacy paths, UNION ALL, subquery+name-collision, NOT/notEquals/NOT IN keep-cases, multi-surviving-additional-segments base-pruned scenario, IN coercion soundness.
• Separate follow-ups:
  • Fix Range::contains(FieldRef) in src/Core/Range.cpp (or deprecate it).
  • Add tryDeserializeWholeText-based validation for watermark settings on assignment, not only at pruning time.
  • Consider extracting the additional_query_infos local-plan loop into a helper shared with ClusterProxy::executeQuery.

Tests to add or strengthen

• enable_analyzer = 0 and = 1 variants of every existing pruning test; without both, B1 cannot be detected.
• WHERE NOT (ts <= '…') (NOT-peel correctness), notEquals, NOT IN, IN () — assert keep (regression guard against future support).
• Mixed-type / lossy coercion: customerid = -1, customerid = 1.5, ts > 0, Decimal IN-list crossing scale boundaries.
• Subquery covers: WHERE ts IN (SELECT …) (must keep), SELECT … FROM (SELECT … FROM hybrid) WHERE ts <= '…' with column-name reuse (must keep).
• UNION ALL with broader second arm — assert all rows survive.
• Three-OR-group and >MAX_TOTAL_BRANCHES cases — assert keep.
• Base pruned + ≥2 additional segments surviving (UnionStep path with plans.size() > 1).
• Real multi-host cluster (not loopback) — base-pruned query reading from a genuine remote additional segment.
• Concurrency smoke: query in flight while ALTER MODIFY SETTING hybrid_watermark_* runs — verdict stays stable.

Coverage summary

• Entry points reviewed: getStorageSnapshot, getQueryProcessingStage, read, computeHybridPruningVerdict, substituteHybridWatermarks, canPruneHybridSegment, atom extraction & DNF expansion.
• Transitions reviewed: snapshot freeze → stage decision → cluster mutation → read planning; analyzer vs legacy branches; all-pruned vs base-pruned vs additional-pruned subcases.
• Fault categories: malformed input, missing watermark, exception under fail-open, cancellation, deeply-nested AST, lossy coercion, identifier shadowing, JOIN/UNION/subquery shapes, mixed-version cluster (no protocol changes), concurrent ALTER.
• Deferred / not covered: real remote-host cluster behavior in the new local-plan branch; analyzer path through the pruner end-to-end.
• Main assumptions: StorageSnapshot is constructed once per query in the standard read path; segments and base_segment_predicate are immutable post-construction (unverified).

Final verdict

Status: request changes

Minimum required actions:

• Resolve B1 (analyzer path) — without this, the feature is largely cosmetic in modern deployments.
• Resolve B2 (remote-host correctness in the new local-plan branch) — silent-data-loss risk in real clusters.
• Resolve B3 and B4 (UNION ALL and nested-subquery WHERE leakage) — both can silently drop rows.
• Resolve B5 (use convertFieldToTypeStrict for IN tuples) and B6 (fail-open coverage of substituteHybridWatermarks).
• Add tests that explicitly exercise: both analyzer settings, UNION ALL, nested-subquery name collision, lossy IN coercion, base-pruned with ≥2 surviving additional segments, and a multi-host cluster.
• Address M1 (justify or replace with KeyCondition) and M2/M3 (single verdict, attached to per-query state, not StorageSnapshot).

Superceded by #1788